client/internal/updatemanager/installer/installer_common.go (2)

119-121: Consider adding contextual logging for updater process start failure.

While the error is returned, adding a log statement with context (e.g., command path, arguments) would aid troubleshooting when the updater process fails to start.

 	// Start the updater process asynchronously
 	if err := updateCmd.Start(); err != nil {
+		log.Errorf("failed to start updater process %s: %v", updaterPath, err)
 		return err
 	}

---

127-129: Consider using defer for Process.Release() to guarantee cleanup.

Although there's no error path after Release() in this method, using defer immediately after checking updateCmd.Process would be more robust and follow Go idioms for resource cleanup.

 	pid := updateCmd.Process.Pid
 	log.Infof("updater started with PID %d", pid)

 	// Release the process so the OS can fully detach it
-	if err := updateCmd.Process.Release(); err != nil {
-		log.Warnf("failed to release updater process: %v", err)
-	}
+	defer func() {
+		if err := updateCmd.Process.Release(); err != nil {
+			log.Warnf("failed to release updater process: %v", err)
+		}
+	}()

 	return nil

client/internal/updatemanager/downloader/downloader_test.go (1)

14-199: Downloader tests give solid coverage of happy and error paths

The suite nicely exercises first-try success, retry success/failure, context cancellation during the retry delay, invalid URL, invalid destination, and the retryDelay == 0 no‑retry path using httptest.Server and atomic.Int32. This gives good confidence in DownloadToFile’s behavior.

If you want to trim a bit of repetition, you could optionally factor out helpers for the common TempDir/dstFile setup and the “always 500” server pattern, but it’s not strictly necessary.

client/internal/updatemanager/downloader/downloader.go (3)

100-107: Improve sleepWithContext to avoid dangling time.After timers

Using time.After in a select like this can leave a timer/goroutine around when the context is canceled before the duration elapses. It’s small but avoidable overhead, especially if this is called frequently.

You can use an explicit time.Timer instead so the timer is stopped/drained when the context is done:

-func sleepWithContext(ctx context.Context, duration time.Duration) error {
-	select {
-	case <-time.After(duration):
-		return nil
-	case <-ctx.Done():
-		return ctx.Err()
-	}
-}
+func sleepWithContext(ctx context.Context, duration time.Duration) error {
+	timer := time.NewTimer(duration)
+	defer timer.Stop()
+
+	select {
+	case <-timer.C:
+		return nil
+	case <-ctx.Done():
+		// Ensure the timer channel is drained if it has already fired.
+		if !timer.Stop() {
+			select {
+			case <-timer.C:
+			default:
+			}
+		}
+		return ctx.Err()
+	}
+}

---

109-137: Clarify/strengthen behavior when response size exceeds limit in DownloadToMemory

Right now, DownloadToMemory uses io.ReadAll(io.LimitReader(resp.Body, limit)). If the server sends more than limit bytes, the reader simply returns EOF at limit, and this function succeeds with truncated data. That may be fine, but it’s somewhat ambiguous: callers can’t tell if they got a full response or a truncated one.

If you intend limit to be a hard upper bound, consider reading limit+1 bytes and failing when the bound is exceeded, e.g.:

-	data, err := io.ReadAll(io.LimitReader(resp.Body, limit))
-	if err != nil {
-		return nil, fmt.Errorf("failed to read response body: %w", err)
-	}
-
-	return data, nil
+	data, err := io.ReadAll(io.LimitReader(resp.Body, limit+1))
+	if err != nil {
+		return nil, fmt.Errorf("failed to read response body: %w", err)
+	}
+	if int64(len(data)) > limit {
+		return nil, fmt.Errorf("response too large: exceeds %d bytes", limit)
+	}
+
+	return data, nil

(You might also want to define behavior for limit <= 0, either rejecting it or treating it as “no limit”.)

---

70-80: Optionally allow a configurable HTTP client instead of hard‑coding http.DefaultClient

Both downloadToFileOnce and DownloadToMemory currently bind directly to http.DefaultClient. That’s fine for now, but you may eventually want to:

• Set custom timeouts, proxy, or TLS configuration for updater downloads.
• Inject a custom client in tests (e.g., with lower timeouts or instrumentation).

A lightweight approach is to expose a package‑level client that callers can override in tests or at startup:

 var (
-    // existing imports...
+    // HTTPClient is used for all downloader HTTP requests. It defaults to http.DefaultClient.
+    HTTPClient = http.DefaultClient
 )
 
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := HTTPClient.Do(req)

and similarly in DownloadToMemory. Call sites can then swap in a tuned client when needed.

Also applies to: 109-120

client/ui/update_windows.go (1)

12-44: Clarify semantics of “parent” vs “another” UI process

nbprocess.IsAnotherProcessRunning returns any other UI process with the same name/user, not necessarily a strict parent. The current behavior is fine, but it might be worth updating the comment/log message to reflect that it kills “another running UI instance” to avoid confusion when debugging.

client/ui/event_handler.go (1)

183-275: Log non‑ExitError failures from runSelfCommand

The new variadic runSelfCommand and the --<command>=true argument building look fine, and the updated call sites are consistent. However, if cmd.Run() fails with a non‑*exec.ExitError (e.g., failure to start, context error), the error is currently swallowed.

Consider logging those cases as well:

	if err := cmd.Run(); err != nil {
		var exitErr *exec.ExitError
		if errors.As(err, &exitErr) {
			log.Printf("command '%s' failed with exit code %d", cmd.String(), exitErr.ExitCode())
-		}
-		return
+		} else {
+			log.Printf("command '%s' failed: %v", cmd.String(), err)
+		}
+		return
	}

client/ui/update.go (1)

19-99: Confirm treating RPC failures as “success” for closing the UI is intentional

In the installer‑result goroutine, both getSrvClient and GetInstallerResult failures log and then close(resultOkCh). In the watcher goroutine, any resultOkCh signal leads to:

log.Info("backend exited, upgrade in progress, closing all UI")
killParentUIProcess()
s.app.Quit()

This means transient RPC/connectivity errors are indistinguishable from a clean “installer finished” path and will close the UI as if the upgrade completed.

If that’s not strictly intended, consider instead sending a mapped error to resultErrCh for hard failures and reserving resultOkCh for confirmed installer success (or at least for well‑defined “daemon intentionally stopped for upgrade” states).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

client/internal/updatemanager/installer/installer_run_windows.go (2)

18-27: Download URLs point to personal fork repository.

The msiDownloadURL and exeDownloadURL constants reference github.com/mlsmaycon/netbird instead of the official repository. This will cause production builds to fetch installers from a personal fork.

This was flagged in a previous review. Please update these URLs to point to the official repository or package host before merging.

---

204-213: Architecture mapping may be needed for artifact naming.

The function uses runtime.GOARCH directly without mapping to artifact naming conventions. If the release artifacts use different naming (e.g., x64 instead of amd64), this will cause 404 errors.

This was mentioned in the previous review regarding URL handling. Please verify the artifact naming convention matches runtime.GOARCH values or add appropriate mapping.

client/internal/updatemanager/installer/installer_run_windows.go (2)

173-173: Use named constants for Windows process creation flags.

The magic numbers make the code harder to understand. Consider defining named constants for clarity and maintainability.

+const (
+	createNewProcessGroup     = 0x00000200
+	detachedProcess           = 0x00000008
+	createUnicodeEnvironment  = 0x00000400
+)
+
 func (u *Installer) startUIAsUser(daemonFolder string) error {
 	// ...
-	creationFlags := uint32(0x00000200 | 0x00000008 | 0x00000400) // CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_UNICODE_ENVIRONMENT
+	creationFlags := uint32(createNewProcessGroup | detachedProcess | createUnicodeEnvironment)

---

192-198: Consider using defer for process handle cleanup.

If an error occurs between the two CloseHandle calls (though unlikely given only warnings are logged), the thread handle could leak. Using defer would be more idiomatic and safer.

 	if err != nil {
 		return fmt.Errorf("CreateProcessAsUser failed: %w", err)
 	}

-	// Close handles
-	if err := windows.CloseHandle(pi.Process); err != nil {
-		log.Warnf("failed to close process handle: %v", err)
-	}
-	if err := windows.CloseHandle(pi.Thread); err != nil {
-		log.Warnf("failed to close thread handle: %v", err)
-	}
+	// Close handles - defer in reverse order of importance
+	defer func() {
+		if err := windows.CloseHandle(pi.Thread); err != nil {
+			log.Warnf("failed to close thread handle: %v", err)
+		}
+	}()
+	defer func() {
+		if err := windows.CloseHandle(pi.Process); err != nil {
+			log.Warnf("failed to close process handle: %v", err)
+		}
+	}()

client/internal/updatemanager/installer/installer_common.go (2)

118-128: Process handle release should use defer pattern for safety.

As noted in the PR objectives, calling Process.Release() without defer risks leaked process handles if intermediate errors occur. While unlikely here since it's near the end of the function, using defer is more defensive.

 	// Start the updater process asynchronously
 	if err := updateCmd.Start(); err != nil {
 		return err
 	}

 	pid := updateCmd.Process.Pid
 	log.Infof("updater started with PID %d", pid)

-	// Release the process so the OS can fully detach it
-	if err := updateCmd.Process.Release(); err != nil {
-		log.Warnf("failed to release updater process: %v", err)
-	}
+	// Release the process so the OS can fully detach it
+	defer func() {
+		if err := updateCmd.Process.Release(); err != nil {
+			log.Warnf("failed to release updater process: %v", err)
+		}
+	}()

 	return nil

---

156-159: Early return discards accumulated errors.

If os.ReadDir fails, the error from removing the updater binary (accumulated in merr at line 152-154) is lost. Consider combining the errors.

 	entries, err := os.ReadDir(u.tempDir)
 	if err != nil {
-		return err
+		merr = multierror.Append(merr, fmt.Errorf("failed to read temp dir: %w", err))
+		return merr.ErrorOrNil()
 	}

client/internal/updatemanager/downloader/downloader.go (3)

21-68: Retry flow is good; consider context-aware early exit and atomic temp-file writes

The overall retry logic and error wrapping look solid. Two small robustness tweaks you might consider:

• If the first attempt fails due to context.Canceled / DeadlineExceeded, you can skip logging “retrying” and skip the retry entirely to avoid misleading logs and unnecessary work.
• For update artifacts, an atomic “download to dstFile.tmp then os.Rename to dstFile” pattern prevents other readers from ever seeing a partially written file.

Example for the context-aware early exit:

-import (
-	"context"
-	"fmt"
+import (
+	"context"
+	"errors"
+	"fmt"
@@
-	// First attempt
-	err = downloadToFileOnce(ctx, url, out)
-	if err == nil {
+	// First attempt
+	err = downloadToFileOnce(ctx, url, out)
+	if err == nil {
 		log.Infof("successfully downloaded file to %s", dstFile)
 		return nil
 	}
 
-	// If retryDelay is 0, don't retry
-	if retryDelay == 0 {
+	// If retryDelay is 0 or context is already done, don't retry
+	if retryDelay == 0 ||
+		errors.Is(err, context.Canceled) ||
+		errors.Is(err, context.DeadlineExceeded) {
 		return err
 	}

---

70-99: Clarify limit semantics and behavior on oversized responses

DownloadToMemory works, but two edge cases are a bit underspecified:

• If limit <= 0, io.LimitReader yields an immediate EOF and you successfully return an empty slice, which can easily mask misconfiguration at call sites.
• If the server sends more than limit bytes, you’ll silently truncate to limit bytes without signaling that the payload was incomplete.

For an updater path, both can be surprising. Consider:

• Either rejecting limit <= 0 (or treating it as “no limit”) explicitly.
• Optionally, failing fast when resp.ContentLength > 0 && resp.ContentLength > limit, so callers get a clear “response too large” error instead of truncated data.

Example of a defensive guard at the top of the function:

func DownloadToMemory(ctx context.Context, url string, limit int64) ([]byte, error) {
+	if limit <= 0 {
+		return nil, fmt.Errorf("limit must be greater than 0, got %d", limit)
+	}

---

101-138: Helpers correctly handle context and resource cleanup

downloadToFileOnce and sleepWithContext look clean: good use of NewRequestWithContext, proper defer for resp.Body.Close(), and straightforward status checks and copy logic. Using sleepWithContext to make the retry delay cancellable is also a nice touch.

One possible future improvement would be to accept an *http.Client as a dependency (rather than using http.DefaultClient) so callers can inject timeouts/proxy settings specific to the update channel, but that’s not strictly necessary for this PR.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

client/internal/updatemanager/installer/installer_run_darwin.go (1)

18-26: Avoid hardcoded temp dir + fork download URL; Type arg is ignored
These were already raised before: defaultTempDir is hardcoded, pkgDownloadURL points at a personal fork, and urlWithVersionArch(_ Type, ...) ignores Type (confusing API surface).

Also applies to: 235-238

client/internal/profilemanager/config.go (1)

3-15: Darwin home resolution: os/user may be brittle depending on build mode—consider os.UserHomeDir() or document why user.Current() is required.
On some setups (notably certain cross-compile/static builds), os/user behavior can be surprising; if you rely on user.Current() to work without $HOME, that’s worth a short comment explaining the rationale.

 import (
 	"context"
 	"crypto/tls"
 	"fmt"
 	"net/url"
 	"os"
-	"os/user"
 	"path/filepath"
 	"reflect"
 	"runtime"
 	"slices"
 	"strings"
 	"time
@@
 )

 func baseConfigDir() (string, error) {
 	if runtime.GOOS == "darwin" {
-		if u, err := user.Current(); err == nil && u.HomeDir != "" {
-			return filepath.Join(u.HomeDir, "Library", "Application Support"), nil
+		if home, err := os.UserHomeDir(); err == nil && home != "" {
+			return filepath.Join(home, "Library", "Application Support"), nil
 		}
 	}
 	return os.UserConfigDir()
 }

client/internal/updatemanager/installer/installer_run_darwin.go (1)

142-161: PKG install drops stdout/stderr; add captured output for debugging failures
Right now cmd.Wait() errors don’t include installer output, which will make field debugging painful. You can keep Start/Wait (for your “don’t double-start” constraint) but still capture logs via buffers.

 func (u *Installer) installPkgFile(ctx context.Context, path string) error {
 	log.Infof("installing pkg file: %s", path)
@@
 	cmd := exec.CommandContext(ctx, "installer", "-pkg", path, "-target", volume)
+	var out strings.Builder
+	cmd.Stdout = &out
+	cmd.Stderr = &out
 	if err := cmd.Start(); err != nil {
 		return fmt.Errorf("error running pkg file: %w", err)
 	}
 	log.Infof("installer started with PID %d", cmd.Process.Pid)
 	if err := cmd.Wait(); err != nil {
-		return fmt.Errorf("error running pkg file: %w", err)
+		return fmt.Errorf("error running pkg file: %w, output: %s", err, out.String())
 	}
 	log.Infof("pkg file installed successfully")
 	return nil
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

client/internal/updatemanager/manager.go (2)

77-112: Don’t delete persisted update state before emitting the “completed” event (crash window).

Current flow deletes the only evidence (UpdateState) before you decide whether to publish the “Auto-update completed” event. A crash between delete and publish loses the completion notification permanently.

Concrete fix: split “load” from “delete”, publish first, delete after. (This is the same concern raised previously, but it’s still present in this version.)

 func (m *Manager) CheckUpdateSuccess(ctx context.Context) {
@@
-	updateState, err := m.loadAndDeleteUpdateState(ctx)
+	updateState, err := m.loadUpdateState()
 	if err != nil {
 		if errors.Is(err, errNoUpdateState) {
 			return
 		}
 		log.Errorf("failed to load update state: %v", err)
 		return
 	}
@@
 	if updateState.TargetVersion == m.currentVersion {
 		m.statusRecorder.PublishEvent(
@@
 		)
-		return
 	}
+
+	// Best-effort cleanup after emitting the event decision.
+	if err := m.deleteUpdateState(ctx, updateState); err != nil {
+		log.Errorf("failed to delete update state: %v", err)
+	}
 }
+
+func (m *Manager) loadUpdateState() (*UpdateState, error) {
+	stateType := &UpdateState{}
+	if err := m.stateManager.LoadState(stateType); err != nil {
+		return nil, fmt.Errorf("load state: %w", err)
+	}
+	state := m.stateManager.GetState(stateType)
+	if state == nil {
+		return nil, errNoUpdateState
+	}
+	updateState, ok := state.(*UpdateState)
+	if !ok {
+		return nil, fmt.Errorf("failed to cast state to UpdateState")
+	}
+	return updateState, nil
+}
+
+func (m *Manager) deleteUpdateState(ctx context.Context, updateState *UpdateState) error {
+	if err := m.stateManager.DeleteState(updateState); err != nil {
+		return fmt.Errorf("delete state: %w", err)
+	}
+	return m.stateManager.PersistState(ctx)
+}

Also applies to: 91-111, 306-335

---

114-135: Lifecycle (Start/Stop) isn’t concurrency-safe; at least document or guard m.cancel access.

Start()/Stop()/onContextCancel() use m.cancel as a sentinel without any synchronization, so concurrent callers can race (and this was already pointed out in prior review comments). If the design relies on “upper layer synchronization”, it’s worth making that explicit at the type level (godoc) or adding a small mutex around lifecycle transitions to prevent footguns.

Also applies to: 175-203

client/internal/updatemanager/manager.go (2)

61-75: Avoid re-registering UpdateState on every load; validate nil inputs in NewManager.

NewManager() already registers UpdateState, but loadAndDeleteUpdateState() registers again. If RegisterState isn’t strictly idempotent, this can become a subtle bug. Also, statusRecorder / stateManager nil will panic later.

 func NewManager(statusRecorder *peer.Status, stateManager *statemanager.Manager) (*Manager, error) {
+    if statusRecorder == nil {
+        return nil, fmt.Errorf("statusRecorder is nil")
+    }
+    if stateManager == nil {
+        return nil, fmt.Errorf("stateManager is nil")
+    }
 	manager := &Manager{
 		statusRecorder: statusRecorder,
 		stateManager:   stateManager,
@@
 	}
 	manager.triggerUpdateFn = manager.triggerUpdate
 
 	stateManager.RegisterState(&UpdateState{})
 
 	return manager, nil
 }
@@
 func (m *Manager) loadAndDeleteUpdateState(ctx context.Context) (*UpdateState, error) {
 	stateType := &UpdateState{}
 
-	m.stateManager.RegisterState(stateType)
 	if err := m.stateManager.LoadState(stateType); err != nil {
 		return nil, fmt.Errorf("load state: %w", err)
 	}

Also applies to: 72-73, 308-334

---

352-355: Replace magic 5*time.Minute with a named constant.

Keeps behavior identical, but makes policy easier to tune/grep (and aligns with other “update policy” knobs).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

client/internal/updatemanager/installer/types_darwin.go (1)

13-22: Guard against nil ProcessState panic.

The existing past review comment on this issue remains valid. Line 16 dereferences cmd.ProcessState without checking if it's nil, which can panic if cmd.Output() fails before the process starts (e.g., pkgutil is missing or not executable).

Apply this diff to use safe error type checking:

 func TypeOfInstaller(ctx context.Context) Type {
 	cmd := exec.CommandContext(ctx, "pkgutil", "--pkg-info", "io.netbird.client")
 	_, err := cmd.Output()
-	if err != nil && cmd.ProcessState.ExitCode() == 1 {
-		// Not installed using pkg file, thus installed using Homebrew
-
+	if err != nil {
+		if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 {
+			// Not installed using pkg file, thus installed using Homebrew
+			return TypeHomebrew
+		}
+		// For other errors (ctx cancellation, missing pkgutil, etc.), fall back to PKG
 		return TypeHomebrew
 	}
 	return TypePKG
 }

client/internal/updatemanager/installer/installer_run_darwin.go (5)

25-25: Hard-coded personal fork URL is a supply-chain risk.

The previous review comment on this line remains unresolved. The download URL points to github.com/mlsmaycon/netbird (personal fork) instead of the official repository. This is a critical supply-chain and reliability risk for production releases.

Change pkgDownloadURL to point to the canonical NetBird organization repository or make it configurable via a build-time constant or environment variable.

---

74-79: Add default case to prevent silent success on unknown installer type.

The previous review comment on this section remains valid. The switch statement on TypeOfInstaller(ctx) lacks a default case. If an unexpected installer type is returned, resultErr remains nil and the function reports success despite performing no installation.

Apply this diff:

 	switch TypeOfInstaller(ctx) {
 	case TypePKG:
+		if strings.TrimSpace(installerFile) == "" {
+			resultErr = fmt.Errorf("installer file path is empty for PKG installer")
+			return resultErr
+		}
 		resultErr = u.installPkgFile(ctx, installerFile)
 	case TypeHomebrew:
 		resultErr = u.updateHomeBrew(ctx)
+	default:
+		resultErr = fmt.Errorf("unsupported installer type: %v", TypeOfInstaller(ctx))
 	}

---

67-72: Dry-run should not persist as a failure result.

The previous review comment on this section is still applicable. Setting resultErr = fmt.Errorf("dry-run mode enabled") causes the deferred result handler to write this as a failure (line 44). Upstream callers treat this identically to real installation failures, which can trigger false alarms in monitoring systems.

For successful dry-run execution, set resultErr = nil and return immediately:

 	if dryRun {
 		time.Sleep(7 * time.Second)
 		log.Infof("dry-run mode enabled, skipping actual installation")
-		resultErr = fmt.Errorf("dry-run mode enabled")
-		return
+		return nil
 	}

---

113-113: Avoid logging PII (username and HOME directory).

The previous review comment regarding PII in logs remains valid. Lines 113, 127, and 138 log the console username and HOME directory path. If logs are collected or uploaded, this exposes unnecessary personally identifiable information.

Downgrade these logs to Debug level or redact the sensitive values:

-	log.Infof("starting UI for user: %s", username)
+	log.Debugf("starting UI for console user")

-	log.Infof("set HOME environment variable: %s", userInfo.HomeDir)
+	log.Debugf("set HOME environment variable for user session")

-	log.Infof("netbird-ui started successfully for user %s", username)
+	log.Infof("netbird-ui started successfully")

Also applies to: 127-127, 138-138

---

163-222: Add safeguards for Homebrew execution.

The previous review comment on this section remains applicable. Two issues:

1. No root user guard: If the tap directory owner resolves to root (line 192-196), the code will attempt to run Homebrew as root, which Homebrew explicitly discourages and may fail.
2. Missing brew binary check: The code should fail fast if brewBinPath doesn't exist (lines 174, 178).

Apply this diff:

 	if _, err := os.Stat(brewTapPath); os.IsNotExist(err) {
 		// Try Intel Mac path
 		brewTapPath = "/usr/local/Homebrew/Library/Taps/netbirdio/homebrew-tap/"
 		brewBinPath = "/usr/local/bin/brew"
 	}
+
+	// Verify brew binary exists
+	if _, err := os.Stat(brewBinPath); err != nil {
+		return fmt.Errorf("homebrew binary not found at %s: %w", brewBinPath, err)
+	}

 	fileInfo, err := os.Stat(brewTapPath)
 	if err != nil {
 		return fmt.Errorf("error getting homebrew installation path info: %w", err)
 	}

 	fileSysInfo, ok := fileInfo.Sys().(*syscall.Stat_t)
 	if !ok {
 		return fmt.Errorf("error checking file owner, sysInfo type is %T not *syscall.Stat_t", fileInfo.Sys())
 	}

 	// Get username from UID
 	brewUser, err := user.LookupId(fmt.Sprintf("%d", fileSysInfo.Uid))
 	if err != nil {
 		return fmt.Errorf("error looking up brew installer user: %w", err)
 	}
 	userName := brewUser.Username
+	
+	// Refuse to run as root
+	if userName == "root" || userName == "" {
+		return fmt.Errorf("refusing to run Homebrew as user %q (Homebrew should not be run as root)", userName)
+	}

client/internal/updatemanager/manager.go (1)

184-198: Potential race condition in Stop() method.

Line 185 reads m.cancel outside the mutex, then line 189 calls m.cancel(), then lines 190-195 lock the mutex to access m.update. If Stop() is called concurrently with another method that modifies m.cancel or m.update, this can lead to race conditions or nil pointer dereferences.

Consider holding the lock for the entire Stop() operation:

 func (m *Manager) Stop() {
+	m.updateMutex.Lock()
+	defer m.updateMutex.Unlock()
+
 	if m.cancel == nil {
 		return
 	}

 	m.cancel()
-	m.updateMutex.Lock()
 	if m.update != nil {
 		m.update.StopWatch()
 		m.update = nil
 	}
-	m.updateMutex.Unlock()

+	// Release lock before waiting for goroutines
+	m.updateMutex.Unlock()
 	m.wg.Wait()
+	m.updateMutex.Lock()
 }

Alternatively, document that Stop() must not be called concurrently and add a note that upper-layer synchronization is required (as mentioned in past review responses).

client/internal/updatemanager/installer/installer_common.go (1)

211-217: Harden temp directory permissions.

Line 212 creates the temp directory with 0o755 permissions, making it world-readable. Since the updater binary is copied here and later executed with elevated privileges, consider using stricter permissions (0o700) to prevent unauthorized access or tampering.

Apply this diff:

 func (u *Installer) mkTempDir() error {
-	if err := os.MkdirAll(u.tempDir, 0o755); err != nil {
+	if err := os.MkdirAll(u.tempDir, 0o700); err != nil {
 		log.Debugf("failed to create tempdir: %s", u.tempDir)
 		return err
 	}

client/internal/updatemanager/installer/installer_run_darwin.go (1)

235-238: Remove unused Type parameter or incorporate it into URL logic.

The urlWithVersionArch function accepts a Type parameter but ignores it (line 235). Either use it to select different download channels/paths, or remove it to avoid confusion.

Apply this diff to remove the unused parameter:

-func urlWithVersionArch(_ Type, version string) string {
+func urlWithVersionArch(version string) string {
 	url := strings.ReplaceAll(pkgDownloadURL, "%version", version)
 	return strings.ReplaceAll(url, "%arch", runtime.GOARCH)
 }

Then update the caller in installer_common.go (line 181) accordingly.

client/internal/updatemanager/manager.go (1)

354-357: Magic number should be a named constant.

The 5-minute throttle interval (line 354) is hard-coded. Consider defining it as a package-level constant for easier maintenance and testing.

Apply this diff:

+const (
+	minUpdateInterval = 5 * time.Minute
+)
+
 func (m *Manager) shouldUpdate(updateVersion *v.Version) bool {
 	// ... existing checks ...
 
-	if time.Since(m.lastTrigger) < 5*time.Minute {
+	if time.Since(m.lastTrigger) < minUpdateInterval {
 		log.Debugf("skipping auto-update, last update was %s ago", time.Since(m.lastTrigger))
 		return false
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.247Z
Learning: In client/ssh/server/executor_unix.go, the method ExecuteWithPrivilegeDrop(ctx context.Context, config ExecutorConfig) has a void return type (no error return). It handles failures by exiting the process directly with appropriate exit codes rather than returning errors to the caller.

client/internal/updatemanager/manager.go (3)

23-27: Consider adding a constant for the update debounce duration.

Line 357 uses 5*time.Minute as a hardcoded duration to prevent rapid successive updates. For consistency and maintainability, consider defining this as a constant here.

 const (
 	latestVersion = "latest"
 	// this version will be ignored
 	developmentVersion = "development"
+	// updateDebounceInterval prevents triggering updates more frequently than this duration
+	updateDebounceInterval = 5 * time.Minute
 )

Then update line 357:

-	if time.Since(m.lastTrigger) < 5*time.Minute {
+	if time.Since(m.lastTrigger) < updateDebounceInterval {

---

56-57: Update comment to reflect all fields protected by the mutex.

The mutex also protects the updateToLatestVersion field (lines 166, 178, 243), not just update and expectedVersion.

-	// updateMutex protect update and expectedVersion fields
+	// updateMutex protects update, expectedVersion, and updateToLatestVersion fields
 	updateMutex sync.Mutex

---

169-173: Consider clearing state consistently on parse errors.

When version parsing fails, the method returns early but leaves m.expectedVersion and m.updateToLatestVersion in their previous state. While the invalid version is rejected, this creates an asymmetry with the empty-string case (lines 158-163), which explicitly clears both fields.

For consistency, consider:

 		expectedSemVer, err := v.NewVersion(expectedVersion)
 		if err != nil {
 			log.Errorf("error parsing version: %v", err)
+			m.expectedVersion = nil
+			m.updateToLatestVersion = false
 			return
 		}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

management/server/http/handlers/accounts/accounts_handler.go (1)

249-252: Critical issue fixed: return statement now present.

The missing return statement after error handling has been added, preventing the nil pointer dereference issue that was flagged in previous reviews.

Configuration used: defaults

Review profile: CHILL

Plan: Pro